feat: Rotate Catalyst Servers to Retry #127
Conversation
There was a problem hiding this comment.
LGTM to me so far, it is nice to have the retry mechanism at queue level almost ready, we can see if this rotation of Catalyst fixes the edge cases we are having, otherwise we can go further and finish implementing the retry at queue level or the DLQ as we discussed.
Great work, thanks!
| : contentClient | ||
|
|
||
| return retry(() => contentClientToUse.fetchEntityById(entityId), retries, waitTime) | ||
| function getContentClientOrDefault(contentServerUrl?: string): ContentClient { |
There was a problem hiding this comment.
This use case will be very common, we can live with it for now but I think the best thing to do on these cases is to expose a function in catalyst-client's contentClient and lambdaClient like:
function setCatalyst(url: string): voidThere was a problem hiding this comment.
| @@ -43,10 +41,10 @@ export async function createEventParser({ | |||
| } | |||
|
|
|||
| async function parseCatalystEvent(event: any): Promise<CatalystDeploymentEvent | undefined> { | |||
| const contentUrl = event.contentServerUrls ? event.contentServerUrls[0] : loadBalancer | |||
| const { contentServerUrls } = event | |||
There was a problem hiding this comment.
Will this be always populated? maybe we can have a fallback just in case
| const results = await Promise.all(asyncResults) | ||
|
|
||
| const badgesToGrant = results.filter(({ ok, result }) => ok && !!result).map(({ result }) => result) | ||
| const handlersToRetry = results.filter(({ ok }) => !ok) |
There was a problem hiding this comment.
I would keep it for now but add an underscore so we by-pass the lint warning
| const handlersToRetry = results.filter(({ ok }) => !ok) | |
| const _handlersToRetry = results.filter(({ ok }) => !ok) |
processor/src/types.ts
Outdated
| @@ -127,6 +127,12 @@ export class ParsingEventError extends Error { | |||
| } | |||
| } | |||
|
|
|||
| export type EventHandlerResponse = { | |||
| ok: boolean | |||
| result?: any | |||
There was a problem hiding this comment.
This is the ObserverResponse, right? I'd name like that and may can we join result and error? so result can be either a correct answer (Badge I think) or a Error type
There was a problem hiding this comment.
WDYT about naming it EventProcessingResult? I think this name better reflects the overall outcome of processing an event and aligns more closely with its intended function.
What benefits would we gain by combining both the result and the error? I prefer to keep them separated.
No description provided.